align struct fields to reduce memory usage #3168
Conversation
jycor
force-pushed
the
james/align
branch
4 times, most recently
from
5c73097 to
5074e26
Compare
fulghum
left a comment
fulghum
left a comment
There was a problem hiding this comment.
Changes look good. Several comments are getting dropped that seem worth keeping though.
| // reference expression for boundary calculation | ||
| orderBy sql.Expression | ||
|
|
||
| // boundary arithmetic on [orderBy] for range start value | ||
| // is set unless [unboundedPreceding] is true | ||
| startInclusion sql.Expression | ||
| // boundary arithmetic on [orderBy] for range end value | ||
| // is set unless [unboundedFollowing] is true | ||
| endInclusion sql.Expression |
There was a problem hiding this comment.
Why not keep these comments around? Seems like they could be helpful for new readers of this code.
| // IsPointLookup is true if the lookup will return one or zero | ||
| // values; the range is null safe, the index is unique, every index | ||
| // column has a range expression, and every range expression is an | ||
| // exact equality. |
There was a problem hiding this comment.
Why not keep this comment?
|
|
||
| // JsonTableCol represents a column in a json table. | ||
| type JsonTableCol struct { | ||
| Path string // if there are nested columns, this is a schema Path, otherwise it is a col Path |
There was a problem hiding this comment.
This comment seems like it explains non obvious context
| // EventScheduler is used to notify EventSchedulerStatus of database deletion, | ||
| // so the events of this database in the scheduler will be removed. |
There was a problem hiding this comment.
Why not keep this comment? Doesn't cost anything and may help future maintainers understand the intent of the code faster.
|
|
||
| name string | ||
| TextDefinition string | ||
| // OuterScopeVisibility is true when a SubqueryAlias (i.e. derived table) is contained in a subquery |
There was a problem hiding this comment.
Looks like you cut out the first line of this comment, but left in the second line. Please keep this comment in to help future maintainers!
| UnaryNode | ||
| checks sql.CheckConstraints | ||
| Ignore bool | ||
| // supported in MySQL's syntax, but is exposed through PostgreSQL's syntax. |
There was a problem hiding this comment.
This comment also got screwed up. Please fix
Add a 1-second wait time before each test to ensure the database has started Update error assertions to match more specific error messages Adjust the test order: first test the case where the user does not exist, then test the case where the password is incorrect
Golang does not automatically arrange the fields of a struct to reduce memory usage.
I used fieldalignment to rearrange the member variables of all the structs in GMS so they are more compact. The result should be a lower memory footprint and slightly better performance.
I avoided making changes to the test files as they do not impact our performance.
TODO: